Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor: add test + docs for 2 argument trunc with columns #7042

Merged
merged 7 commits into from
Jul 27, 2023

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Jul 20, 2023

Which issue does this PR close?

Related to #6942 from @Syleechan

Rationale for this change

While reviewing #6942 I noticed the two argument version of trunc wasn't tested via sql and it wasn't documented

What changes are included in this PR?

add test + docs for 2 argument trunc with columns

Are these changes tested?

Yes

Are there any user-facing changes?

@github-actions github-actions bot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Jul 20, 2023
@alamb
Copy link
Contributor Author

alamb commented Jul 25, 2023

@Syleechan would you have time to review this PR?

Comment on lines 921 to 928
# trunc with columns and precision
query RRR rowsort
select trunc(a, 3) as a3, trunc(a, 1) as a1, trunc(arrow_cast(a, 'Float64'), 3) as a3_f64 from small_floats;
----
-0.7 -0.7 -0.7
-1 -1 -1
0.2 0.2 0.2
0.5 0.5 0.5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, as a has at most 1 decimal point, trunc(a, 3) or trunc(a, 1) basically cannot show the difference. I think it doesn't actually test what we want?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add some long decimal point float number

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

Comment on lines 510 to 519
trunc(numeric_expression[, decimal_places])
```

#### Arguments

- **numeric_expression**: Numeric expression to operate on.
Can be a constant, column, or function, and any combination of arithmetic operators.

- **decimal_places**: Optional. The number of decimal places to truncate to.
Defaults to 0.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc looks good to me. Just one question. Can decimal_places be negative?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can not be negative, modify the trunc function just meet the self-defined precision of decimal to '.'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL turns out that negative precision means "truncate to the left of the decimal place": https://www.postgresqltutorial.com/postgresql-math-functions/postgresql-trunc/#:~:text=If%20the%20precision%20argument%20is,The%20precision%20argument%20is%20optional.

postgres=# select trunc(12345.6789, -2);
 trunc
-------
 12300
(1 row)

This is actually what the current datafusion implementation does too so I have added a test and updated the docs

Copy link
Contributor

@Syleechan Syleechan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @alamb , I left some comments.

Comment on lines 921 to 928
# trunc with columns and precision
query RRR rowsort
select trunc(a, 3) as a3, trunc(a, 1) as a1, trunc(arrow_cast(a, 'Float64'), 3) as a3_f64 from small_floats;
----
-0.7 -0.7 -0.7
-1 -1 -1
0.2 0.2 0.2
0.5 0.5 0.5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add some long decimal point float number

Comment on lines 510 to 519
trunc(numeric_expression[, decimal_places])
```

#### Arguments

- **numeric_expression**: Numeric expression to operate on.
Can be a constant, column, or function, and any combination of arithmetic operators.

- **decimal_places**: Optional. The number of decimal places to truncate to.
Defaults to 0.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can not be negative, modify the trunc function just meet the self-defined precision of decimal to '.'

@alamb
Copy link
Contributor Author

alamb commented Jul 26, 2023

Thanks @viirya and @Syleechan - I have addressed your comments.

Copy link
Contributor

@Syleechan Syleechan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Approved.

@alamb alamb merged commit a3088be into apache:main Jul 27, 2023
@alamb alamb deleted the arithmetic_truncate branch July 27, 2023 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants